fix(dreamer): gate and scan the retrospective on real message activity - #464
TreedsSlop wants to merge 1 commit into
Conversation
The retrospective gate tested `session_projects.updated_at > watermark`, but updated_at is written only at first project binding (and by the backfill at scan time) — it is not an activity timestamp: - under-scan: once the watermark passed a session's registration time, new messages in that session could never re-trigger a run, and a session truncated at the per-session cap never had its tail read; - over-scan: a backfilled old session is stamped with the backfill time, newer than every message it contains, so it stayed eligible forever and re-ran nightly to no effect. Derive the activity signal from the message table at query time instead: - a new MessageActivityProvider counts root sessions with a message newer than the watermark (indexed message table, sub-ms); a missing provider or an unavailable store is treated conservatively as "run" (the executor bails before any child session); - the retrospective scanner makes message activity the eligibility driver, demoting the updated_at filter to the indexless-provider fallback.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
6 issues found across 10 files
You’re at about 94% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/plugin/src/features/magic-context/dreamer/task-gates.ts">
<violation number="1" location="packages/plugin/src/features/magic-context/dreamer/task-gates.ts:406">
P1: When `messageActivity` is omitted, this branch falls back to `session_projects.updated_at` and can skip retrospective work instead of allowing the executor to bail safely. Return true whenever the provider is absent; reserve the legacy count only for an explicitly supported indexless gate.</violation>
</file>
<file name="packages/plugin/src/features/magic-context/dreamer/message-activity.ts">
<violation number="1" location="packages/plugin/src/features/magic-context/dreamer/message-activity.ts:50">
P1: When the OpenCode DB opens but its `message` table is unavailable, this read throws instead of returning the documented conservative `null` result. The gate then aborts `runDueTasksForProject`, so the retrospective executor never gets the intended chance to run; catch the activity query error and return `null`.</violation>
</file>
<file name="packages/plugin/src/features/magic-context/dreamer/task-scheduler.ts">
<violation number="1" location="packages/plugin/src/features/magic-context/dreamer/task-scheduler.ts:479">
P2: After a retrospective has a content watermark, manual backlog snapshots count all root sessions because these provider-backed probes omit `retrospectiveWatermarkMs`. Pass the task’s stored watermark to each retrospective backlog probe so `backlogBefore` and `backlogAfter` report pending activity accurately.</violation>
</file>
<file name="packages/plugin/src/features/magic-context/dreamer/retrospective-gate.test.ts">
<violation number="1" location="packages/plugin/src/features/magic-context/dreamer/retrospective-gate.test.ts:220">
P2: The over-scan assertion can't detect s2 being scanned: s2 has no rows past the watermark, so a regression that scans it in addition to s1 leaves win.messages, maxScannedTs, and the every(s1) check unchanged. The test only catches the full old behavior (whose failure comes from s1 being excluded, i.e. the under-scan side). Record reads in ScriptedProvider and assert s2 was never read, or make the regression change the output.</violation>
<violation number="2" location="packages/plugin/src/features/magic-context/dreamer/retrospective-gate.test.ts:228">
P3: This fallback test has no discriminating data: stale is excluded by both updatedAt and any other eligibility rule because it has no messages. The test still passes if the updatedAt filter were dropped entirely. Give stale a message with ts > watermark (e.g. u("stale", 300, ...)) so a broken fallback filter would include it and change win.messages, making the branch it names actually observable.</violation>
</file>
<file name="packages/plugin/src/features/magic-context/dreamer/message-activity.test.ts">
<violation number="1" location="packages/plugin/src/features/magic-context/dreamer/message-activity.test.ts:96">
P2: The gate's eligibility hinges on the strict `time_created > sinceMs` boundary (a message at exactly the watermark must not count), yet no test exercises it: every fixture timestamp is 50+ ms away from `sinceMs`, so a regression to `>=` (over-scanning a session whose only message sits at the watermark) would pass the whole suite. Add a case with a message at `ts == sinceMs` (excluded) and one at `ts == sinceMs + 1` (included), and a session holding both a stale and a fresh message (the WHERE-before-GROUP semantics: any fresh message counts).</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| if (ctx.messageActivity) { | ||
| const count = ctx.messageActivity.countRootSessionsWithMessagesSince( | ||
| project, | ||
| ctx.retrospectiveWatermarkMs ?? null, | ||
| ); | ||
| return count === null ? true : count > 0; | ||
| } | ||
| return countProjectSessionsSince(db, project, ctx.retrospectiveWatermarkMs ?? null) > 0; |
There was a problem hiding this comment.
P1: When messageActivity is omitted, this branch falls back to session_projects.updated_at and can skip retrospective work instead of allowing the executor to bail safely. Return true whenever the provider is absent; reserve the legacy count only for an explicitly supported indexless gate.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/plugin/src/features/magic-context/dreamer/task-gates.ts, line 406:
<comment>When `messageActivity` is omitted, this branch falls back to `session_projects.updated_at` and can skip retrospective work instead of allowing the executor to bail safely. Return true whenever the provider is absent; reserve the legacy count only for an explicitly supported indexless gate.</comment>
<file context>
@@ -384,11 +396,20 @@ export function evaluateTaskGate(task: DreamTaskName, ctx: TaskGateContext): boo
+ // The executor's raw provider does the precise typed-user-message scan
+ // and bails before any child session if empty. Never-run → any root
+ // session; an unavailable message store → conservative allow.
+ if (ctx.messageActivity) {
+ const count = ctx.messageActivity.countRootSessionsWithMessagesSince(
+ project,
</file context>
| if (ctx.messageActivity) { | |
| const count = ctx.messageActivity.countRootSessionsWithMessagesSince( | |
| project, | |
| ctx.retrospectiveWatermarkMs ?? null, | |
| ); | |
| return count === null ? true : count > 0; | |
| } | |
| return countProjectSessionsSince(db, project, ctx.retrospectiveWatermarkMs ?? null) > 0; | |
| if (!ctx.messageActivity) return true; | |
| const count = ctx.messageActivity.countRootSessionsWithMessagesSince( | |
| project, | |
| ctx.retrospectiveWatermarkMs ?? null, | |
| ); | |
| return count === null ? true : count > 0; |
| return readOpenCodeOldestMessageTimesSince( | ||
| db, | ||
| sessions.map((s) => s.session_id), | ||
| sinceMs, | ||
| ).size; |
There was a problem hiding this comment.
P1: When the OpenCode DB opens but its message table is unavailable, this read throws instead of returning the documented conservative null result. The gate then aborts runDueTasksForProject, so the retrospective executor never gets the intended chance to run; catch the activity query error and return null.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/plugin/src/features/magic-context/dreamer/message-activity.ts, line 50:
<comment>When the OpenCode DB opens but its `message` table is unavailable, this read throws instead of returning the documented conservative `null` result. The gate then aborts `runDueTasksForProject`, so the retrospective executor never gets the intended chance to run; catch the activity query error and return `null`.</comment>
<file context>
@@ -0,0 +1,62 @@
+ if (!db) return null;
+ const sessions = selectProjectSessions(deps.contextDb, projectIdentity);
+ if (sinceMs === null) return sessions.length;
+ return readOpenCodeOldestMessageTimesSince(
+ db,
+ sessions.map((s) => s.session_id),
</file context>
| return readOpenCodeOldestMessageTimesSince( | |
| db, | |
| sessions.map((s) => s.session_id), | |
| sinceMs, | |
| ).size; | |
| try { | |
| return readOpenCodeOldestMessageTimesSince( | |
| db, | |
| sessions.map((s) => s.session_id), | |
| sinceMs, | |
| ).size; | |
| } catch { | |
| return null; | |
| } |
|
|
||
| const selectedTaskNames = selected.map((config) => config.task); | ||
| result.backlogBefore = getDreamTaskBacklogs(deps.db, deps.projectIdentity, selectedTaskNames); | ||
| result.backlogBefore = getDreamTaskBacklogs(deps.db, deps.projectIdentity, selectedTaskNames, { |
There was a problem hiding this comment.
P2: After a retrospective has a content watermark, manual backlog snapshots count all root sessions because these provider-backed probes omit retrospectiveWatermarkMs. Pass the task’s stored watermark to each retrospective backlog probe so backlogBefore and backlogAfter report pending activity accurately.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/plugin/src/features/magic-context/dreamer/task-scheduler.ts, line 479:
<comment>After a retrospective has a content watermark, manual backlog snapshots count all root sessions because these provider-backed probes omit `retrospectiveWatermarkMs`. Pass the task’s stored watermark to each retrospective backlog probe so `backlogBefore` and `backlogAfter` report pending activity accurately.</comment>
<file context>
@@ -471,7 +476,9 @@ export async function runManualDream(
const selectedTaskNames = selected.map((config) => config.task);
- result.backlogBefore = getDreamTaskBacklogs(deps.db, deps.projectIdentity, selectedTaskNames);
+ result.backlogBefore = getDreamTaskBacklogs(deps.db, deps.projectIdentity, selectedTaskNames, {
+ messageActivity: deps.messageActivity,
+ });
</file context>
|
|
||
| const win = await readRetrospectiveScanWindow(provider, "proj", 250, 0); | ||
| expect(win.messages.map((m) => m.text)).toEqual(["new1"]); | ||
| expect(win.messages.every((m) => m.sessionId === "s1")).toBe(true); |
There was a problem hiding this comment.
P2: The over-scan assertion can't detect s2 being scanned: s2 has no rows past the watermark, so a regression that scans it in addition to s1 leaves win.messages, maxScannedTs, and the every(s1) check unchanged. The test only catches the full old behavior (whose failure comes from s1 being excluded, i.e. the under-scan side). Record reads in ScriptedProvider and assert s2 was never read, or make the regression change the output.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/plugin/src/features/magic-context/dreamer/retrospective-gate.test.ts, line 220:
<comment>The over-scan assertion can't detect s2 being scanned: s2 has no rows past the watermark, so a regression that scans it in addition to s1 leaves win.messages, maxScannedTs, and the every(s1) check unchanged. The test only catches the full old behavior (whose failure comes from s1 being excluded, i.e. the under-scan side). Record reads in ScriptedProvider and assert s2 was never read, or make the regression change the output.</comment>
<file context>
@@ -184,6 +185,60 @@ describe("readRetrospectiveScanWindow", () => {
+
+ const win = await readRetrospectiveScanWindow(provider, "proj", 250, 0);
+ expect(win.messages.map((m) => m.text)).toEqual(["new1"]);
+ expect(win.messages.every((m) => m.sessionId === "s1")).toBe(true);
+ });
+
</file context>
| addMessage(openDb, "sub1", 999); // newest, but a subagent → ignored | ||
| const provider = createMessageActivityProvider({ contextDb, openOpenCodeDb: () => openDb }); | ||
|
|
||
| expect(provider.countRootSessionsWithMessagesSince(PROJECT_IDENTITY, 150)).toBe(2); |
There was a problem hiding this comment.
P2: The gate's eligibility hinges on the strict time_created > sinceMs boundary (a message at exactly the watermark must not count), yet no test exercises it: every fixture timestamp is 50+ ms away from sinceMs, so a regression to >= (over-scanning a session whose only message sits at the watermark) would pass the whole suite. Add a case with a message at ts == sinceMs (excluded) and one at ts == sinceMs + 1 (included), and a session holding both a stale and a fresh message (the WHERE-before-GROUP semantics: any fresh message counts).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/plugin/src/features/magic-context/dreamer/message-activity.test.ts, line 96:
<comment>The gate's eligibility hinges on the strict `time_created > sinceMs` boundary (a message at exactly the watermark must not count), yet no test exercises it: every fixture timestamp is 50+ ms away from `sinceMs`, so a regression to `>=` (over-scanning a session whose only message sits at the watermark) would pass the whole suite. Add a case with a message at `ts == sinceMs` (excluded) and one at `ts == sinceMs + 1` (included), and a session holding both a stale and a fresh message (the WHERE-before-GROUP semantics: any fresh message counts).</comment>
<file context>
@@ -0,0 +1,137 @@
+ addMessage(openDb, "sub1", 999); // newest, but a subagent → ignored
+ const provider = createMessageActivityProvider({ contextDb, openOpenCodeDb: () => openDb });
+
+ expect(provider.countRootSessionsWithMessagesSince(PROJECT_IDENTITY, 150)).toBe(2);
+ provider.dispose();
+ });
</file context>
| // filter is their only eligibility signal and must still exclude stale | ||
| // sessions (registration time ≤ watermark). | ||
| const provider: RetrospectiveRawProvider = { | ||
| listProjectSessions: () => [ |
There was a problem hiding this comment.
P3: This fallback test has no discriminating data: stale is excluded by both updatedAt and any other eligibility rule because it has no messages. The test still passes if the updatedAt filter were dropped entirely. Give stale a message with ts > watermark (e.g. u("stale", 300, ...)) so a broken fallback filter would include it and change win.messages, making the branch it names actually observable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/plugin/src/features/magic-context/dreamer/retrospective-gate.test.ts, line 228:
<comment>This fallback test has no discriminating data: stale is excluded by both updatedAt and any other eligibility rule because it has no messages. The test still passes if the updatedAt filter were dropped entirely. Give stale a message with ts > watermark (e.g. u("stale", 300, ...)) so a broken fallback filter would include it and change win.messages, making the branch it names actually observable.</comment>
<file context>
@@ -184,6 +185,60 @@ describe("readRetrospectiveScanWindow", () => {
+ // filter is their only eligibility signal and must still exclude stale
+ // sessions (registration time ≤ watermark).
+ const provider: RetrospectiveRawProvider = {
+ listProjectSessions: () => [
+ { sessionId: "active", updatedAt: 500 },
+ { sessionId: "stale", updatedAt: 100 },
</file context>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const messageActivity = createMessageActivityProvider({ contextDb: db, openOpenCodeDb }); | ||
| try { | ||
| const ran = await runDueTasksForProject({ | ||
| db, | ||
| projectIdentity: reg.projectIdentity, | ||
| tasks: runtimeConfigs, | ||
| executor, | ||
| messageActivity, |
There was a problem hiding this comment.
If opencode.db is accessible during a Pi or OMP registration, this OpenCode-specific provider finds no sessions because their project rows use the pi or omp harness. It returns 0 rather than null, so the activity gate skips the retrospective even when the Pi JSONL source contains new messages. Only attach this provider to OpenCode registrations, or supply an activity provider for each harness.
| const oldestBySession = provider.readOldestMessageTimesSince | ||
| ? await provider.readOldestMessageTimesSince( | ||
| eligibleSessions.map(({ session }) => session.sessionId), | ||
| allSessions.map((session) => session.sessionId), | ||
| watermarkMs, | ||
| ) |
There was a problem hiding this comment.
The eligibility query and later per-session reads use separate database snapshots. If another OpenCode process inserts a message after this query into a session missing from oldestBySession, and another retained message has the same millisecond timestamp, the scan can persist that timestamp without reading the new message. The next scan uses time_created > watermark, so it never reads the omitted row. Use a race-safe upper boundary or a watermark tie-breaker that cannot advance past messages inserted between these statements.
1 participant
The retrospective gate tested
session_projects.updated_at > watermark, but updated_at is written only at first project binding (and by the backfill at scan time) — it is not an activity timestamp:Derive the activity signal from the message table at query time instead:
I'm not a huge fan of the "read the opencode DB everywhere" pattern and this fix does not address the issue on Pi, but it is one option for a fix, so I thought I'd offer it as a PR. An alternative might be to put an actually useful activity timestamp in the DB. The downside of that is that one has to always keep that timestamp in sync. For that option, three requirements would have to be met for the timestamp:
(a) Same time base as the watermark — the session's max message time_created, not wall-clock at registration.
(b) Advanced on every activity, not once at binding.
(c) Existing rows repaired — otherwise every already-registered stale session keeps its wrong value forever.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Fixes the retrospective gate and scanner so eligibility is driven by real message activity instead of
session_projects.updated_at, which records first project binding/backfill time, not activity. That caused under-scan (sessions with new messages never re-triggered once the watermark passed their registration time) and over-scan (backfilled old sessions re-ran nightly). A newMessageActivityProvidercounts root sessions with messages newer than the content watermark directly from the message table; a missing provider or unavailable store is conservatively treated as "run", and providers without the indexed frontier keep theupdated_atfilter. This fix does not cover the Pi store.Bug Fixes
Written for commit 785bcc3. Summary will update on new commits.
This PR is not yet safe to merge because it can suppress Pi/OMP retrospectives and can permanently miss a message inserted during an OpenCode scan.
Findings
Summary
This PR replaces registration-time retrospective eligibility with activity derived from OpenCode message timestamps and threads the new activity provider through scheduling paths.
Diagram
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Retrospective becomes due] --> B[Message activity gate] B --> C[Select project root sessions] C --> D[Query message timestamps newer than watermark] D -->|No activity| E[Skip until next schedule] D -->|Activity| F[Acquire retrospective lease] F --> G[Query oldest frontier again] G --> H[Read bounded eligible sessions] H --> I[Normalize and globally cap messages] I --> J[Persist maximum safely scanned timestamp] K[Pi/OMP registration] -. currently receives OpenCode provider .-> B L[Concurrent message insertion] -. can occur between G and H .-> JReviews (1) · Last reviewed commit: "fix(dreamer): gate and scan the retrospe..."